Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add assert_regex #43

Merged
merged 8 commits into from
May 29, 2022

Conversation

rico-chet
Copy link

@rico-chet rico-chet commented May 25, 2022

Add a function wrapping the commonly used [[ value =~ pattern ]]
construct, printing both operands in case of failure.

This addresses the need in #7 without affecting backward compatibility
of assert_equal.

@martin-schulze-vireso
Copy link
Member

martin-schulze-vireso commented May 25, 2022

I am unsure about potential code sharing with assert_output --regex. Then again its not even 20 lines...

@martin-schulze-vireso
Copy link
Member

Pleased add a test for the invalid regex branch.

src/assert_regex.bash Outdated Show resolved Hide resolved
@rico-chet rico-chet force-pushed the assert-equal-regexp branch from c28cb7b to 75c4111 Compare May 26, 2022 12:12
rico-chet added 3 commits May 26, 2022 15:51
Test the new `assert_regex` function.
Add a convenient version of `[[ =~ ]]`.
@rico-chet rico-chet force-pushed the assert-equal-regexp branch from b777b66 to 4f8544b Compare May 26, 2022 13:52
@rico-chet
Copy link
Author

I added the error case test and fixed a copy-paste issue.

I also found that the multi-line value case needs more test coverage and documentation. It's not obvious that the assert succeeds when any line of the value matches the pattern.

@martin-schulze-vireso
Copy link
Member

It's not obvious that the assert succeeds when any line of the value matches the pattern.

I would have thought that it is default regex behavior to Match anywhere in the string If there are no begin/end markers ($^).

@rico-chet rico-chet force-pushed the assert-equal-regexp branch 3 times, most recently from c050578 to ddf9011 Compare May 26, 2022 19:38
@rico-chet
Copy link
Author

It's not obvious that the assert succeeds when any line of the value matches the pattern.

I would have thought that it is default regex behavior to Match anywhere in the string If there are no begin/end markers ($^).

This is the case, but I don't know by heart how multi-line stuff is handled exactly, so I added a couple characterization tests.

@rico-chet
Copy link
Author

I am unsure about potential code sharing with assert_output --regex. Then again its not even 20 lines...

assert_output only acts on the $output content, but not on e.g. $stderr or any other data like a call trace from a mock script (the scenario where I used this last time). This is the rationale for not re-using assert_output for this. Also, I don't like all the parameters as they clutter tests in the long run.

@rico-chet rico-chet force-pushed the assert-equal-regexp branch 2 times, most recently from 73f9021 to bb7d116 Compare May 26, 2022 19:54
@rico-chet
Copy link
Author

should be good to go now

README.md Outdated
If the value is longer than one line then it is displayed in *multi-line*
format.

Multi-line value matches a single-line pattern when one of its lines matches,
Copy link
Member

@martin-schulze-vireso martin-schulze-vireso May 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find that description a bit confusing. This sounds like matching of multiline strings works per line. The "single line pattern" wording might be misleading, as a pattern might match across linebreaks without containing an obvious \n.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get what you mean (\n in the value matches . in the pattern) and I was confused as I don't use multi-line matching often. The message needs an improvement, I agree. I will see how fast I can do it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am planning to simply refer to the Bash docs: https://www.gnu.org/software/bash/manual/html_node/Conditional-Constructs.html#:~:text=[[…]], as it's rather complicated. The applied case sensitivity should also be made visible to users, IMHO. BASH_REMATCH visibility should be part of a test, too.

@martin-schulze-vireso
Copy link
Member

Should we also provide refute_regex in this PR?

@rico-chet
Copy link
Author

Should we also provide refute_regex in this PR?

I prefer a separate PR, to keep the batch size small and get things done faster.

@rico-chet rico-chet marked this pull request as draft May 28, 2022 12:46
@rico-chet rico-chet force-pushed the assert-equal-regexp branch from bb7d116 to d950f28 Compare May 28, 2022 21:27
@rico-chet rico-chet marked this pull request as ready for review May 28, 2022 21:27
@martin-schulze-vireso martin-schulze-vireso merged commit 6ad25d9 into bats-core:master May 29, 2022
@martin-schulze-vireso
Copy link
Member

Thanks for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants